Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: cron jobs #1155

Merged
merged 17 commits into from
Apr 15, 2024
Merged

feat: cron jobs #1155

merged 17 commits into from
Apr 15, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Apr 2, 2024

  • Schema changes
  • Module validation
  • Support multiple controllers using hash ring
  • DB indexes
  • Tests
    • CronJobs Service
    • cron lib
    • schema changes

Separating into different PRs:

#1141

@alecthomas alecthomas mentioned this pull request Apr 2, 2024
@matt2e matt2e force-pushed the matt2e/cron-jobs branch 8 times, most recently from f4c3171 to 0f50c28 Compare April 4, 2024 22:28
@alecthomas
Copy link
Collaborator

Mind breaking this up into multiple PRs? Schema changes could be one, for example.

@alecthomas
Copy link
Collaborator

Database changes could be another, and so on.

matt2e added a commit that referenced this pull request Apr 8, 2024
Built a cron implementation which will be used as part of our cron jobs
feature (See: #1155)
Splitting this off into this smaller PR to make things easier.
matt2e added a commit that referenced this pull request Apr 8, 2024
Previously a deepcopy involving nil pointers would end up with a pointer
to a zero value.

Needed as part of #1155
@matt2e matt2e force-pushed the matt2e/cron-jobs branch 5 times, most recently from 277571f to ac3efcf Compare April 11, 2024 02:44
@matt2e matt2e force-pushed the matt2e/cron-jobs branch 7 times, most recently from b174655 to 2716ff7 Compare April 12, 2024 05:59
@matt2e matt2e marked this pull request as ready for review April 12, 2024 06:52
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome @matt2e.

backend/controller/controller.go Outdated Show resolved Hide resolved
backend/controller/controller.go Show resolved Hide resolved
backend/controller/cronjobs/cronjobs.go Outdated Show resolved Hide resolved
backend/controller/cronjobs/cronjobs.go Outdated Show resolved Hide resolved
backend/controller/cronjobs/cronjobs.go Outdated Show resolved Hide resolved
start := s.clock.Now().UTC()
pattern, err := cron.Parse(stale.Schedule)
if err != nil {
logger.Errorf(err, "Could not kill stale cron job %q because schedule could not be parsed: %q", stale.Key, stale.Schedule)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - all of these log messages should be associated with the cron job in the events table so that the user can filter and find issues with their cron jobs.

backend/controller/cronjobs/cronjobs.go Show resolved Hide resolved
backend/controller/cronjobs/cronjobs.go Outdated Show resolved Hide resolved
backend/controller/cronjobs/cronjobs_test.go Show resolved Hide resolved
backend/controller/cronjobs/cronjobs_test.go Show resolved Hide resolved
backend/controller/cronjobs/state.go Outdated Show resolved Hide resolved
backend/controller/controller.go Outdated Show resolved Hide resolved
backend/controller/cronjobs/state.go Outdated Show resolved Hide resolved
@matt2e matt2e requested a review from a team as a code owner April 14, 2024 23:17
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just a few comments. This is awesome!

@matt2e matt2e merged commit 07682f3 into main Apr 15, 2024
11 checks passed
@matt2e matt2e deleted the matt2e/cron-jobs branch April 15, 2024 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants